Skip to content

Implement normalize_lexically #134696

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ChrisDenton
Copy link
Member

Implements #134694

This is, I think, the most straightforward implementation I could do, which will hopefully more easily allow experimentation if we decide to change the design here.

@rustbot
Copy link
Collaborator

rustbot commented Dec 23, 2024

r? @workingjubilee

rustbot has assigned @workingjubilee.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Dec 23, 2024
@ChrisDenton ChrisDenton force-pushed the normalize-lexically branch 2 times, most recently from c08079c to 8a3fb66 Compare December 23, 2024 17:44
Comment on lines 3014 to 3021
Component::RootDir | Component::Prefix(_) => return Err(NormalizeError),
Component::CurDir => continue,
Component::ParentDir => {
if lexical.as_os_str().len() == root {
return Err(NormalizeError);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is also a "should never happen" case, isn't it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I remember now. As per the RFC, we need to guard against popping from the path when there's nothing to pop. So a/../.. must error which is simple enough. But the complicating factor here is that base paths like C:\ are split by components as (Prefix, RootDir) whereas in reality you almost always want to treat C:\ as one unit. So it checks if popping would go above the root by just doing a length check. This handles whatever the "root" may be, e.g. C: (without the \) or whatever.

@workingjubilee workingjubilee added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 4, 2025
@bors
Copy link
Collaborator

bors commented Feb 4, 2025

☔ The latest upstream changes (presumably #136533) made this pull request unmergeable. Please resolve the merge conflicts.

@workingjubilee
Copy link
Member

Hmm. This wasn't supposed to get frozen on those two questions, so I'm just going to rebase this and approve it. If I think we maybe should call unreachable!() instead of returning I can just PR that.

@workingjubilee workingjubilee force-pushed the normalize-lexically branch from 8a3fb66 to 2955213 Compare May 2, 2025 21:10
@rust-log-analyzer

This comment has been minimized.

@workingjubilee
Copy link
Member

workingjubilee commented May 2, 2025

ugh, the move of the tests breaks them because they relied on a struct expr

@ChrisDenton ChrisDenton force-pushed the normalize-lexically branch from 2955213 to 138d7ef Compare May 22, 2025 10:35
@ChrisDenton
Copy link
Member Author

Sorry for the delay here, I've kept getting distracted by other things. I've rebased, fixed the tests and squished.

@ChrisDenton ChrisDenton force-pushed the normalize-lexically branch from 138d7ef to 79a1ef4 Compare May 22, 2025 10:45
@rust-log-analyzer

This comment has been minimized.

@ChrisDenton ChrisDenton force-pushed the normalize-lexically branch from 79a1ef4 to 6efb929 Compare May 22, 2025 10:58
@ChrisDenton ChrisDenton force-pushed the normalize-lexically branch from 6efb929 to 8788da1 Compare May 22, 2025 11:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants